Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework fix of terminal width support on MINGW (fixes #290) #291

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rosti-il
Copy link

@rosti-il rosti-il commented Jul 2, 2024

No description provided.

Copy link
Member

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed support for differentiating stdout and stderr, is there any reason for that ?

@rosti-il
Copy link
Author

rosti-il commented Sep 7, 2024

You removed support for differentiating stdout and stderr, is there any reason for that ?

There are two main reasons:

  1. It is very unusual for a Java process to have two different terminals with two different or even equal widths for stdout and stderr.
  2. The tty.exe of MSYS2/MINGW is a trivial program. Almost all that it does is calling for the ttyname() function and then printing the returned C string to the stdout. If you really need to support different terminals for stdout and stderr the right thing to do is adding that functionality into the tty.exe (to be called for example as tty.exe -e) in the Coreutils project and this is a really trivial change.

The MingwSupport.getRedirect() method in the original code is an unnecessarily dirty hack that in most cases simply won't work, because most users will not bother to configure --add-opens java.base/java.lang=ALL-UNNAMED for all their Java software that uses JANSI and in some future version of Java that --add-opens option might simply be not supported anymore because the transition period to the strong encapsulation of the Java Platform Module System (JPMS) will be over.

@gnodet
Copy link
Member

gnodet commented Sep 7, 2024

You removed support for differentiating stdout and stderr, is there any reason for that ?

There are two main reasons:

  1. It is very unusual for a Java process to have two different terminals with two different or even equal widths for stdout and stderr.

Supporting both stdout and stderr is important imho.
Just try

> java -jar jansi-2.4.2-SNAPSHOT.jar | grep  terminal
isatty(STDOUT_FILENO): 0, System.out is *NOT* a terminal
isatty(STDERR_FILENO): 1, System.err is a terminal

That's a pretty common use case for java apps running in terminals.

  1. The tty.exe of MSYS2/MINGW is a trivial program. Almost all that it does is calling for the ttyname() function and then printing the returned C string to the stdout. If you really need to support different terminals for stdout and stderr the right thing to do is adding that functionality into the tty.exe (to be called for example as tty.exe -e) in the Coreutils project and this is a really trivial change.

Well, it calls ttyname on the standard input. So your code won't work if the input comes from a pipe either.

> echo foo | java -jar jansi-2.4.2-SNAPSHOT.jar 

The output should still be usable and report the correct width.

The MingwSupport.getRedirect() method in the original code is an unnecessarily dirty hack that in most cases simply won't work, because most users will not bother to configure --add-opens java.base/java.lang=ALL-UNNAMED for all their Java software that uses JANSI and in some future version of Java that --add-opens option might simply be not supported anymore because the transition period to the strong encapsulation of the Java Platform Module System (JPMS) will be over.

Then we could go to JNI ?

@rosti-il
Copy link
Author

rosti-il commented Sep 7, 2024

You're right, I didn't think about pipes and redirections. So the first reason is not relevant but the second still is. I mean tty.exe could be changed to support stdout and stderr in addition to the already supported stdin.

Another any maybe the best option is indeed JNI. The ttyname() and ttyname_r() libc functions of MSYS2 are available in the msys-2.0.dll. In Cygwing I believe they are available in the cygwin1.dll. Both DLLs reside in a directory that is included into PATH, meaning JANSI doesn't need to know the exact location of these DLLs. So, the already existing jansi_ttyname.c just need to support those DLLs in MSYS2 and Cygwin environments. Maybe another existing JNI code could support those DLLs as well, so JANSI could be used without running both tty.exe and stty.exe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants